Reject a Harper node where a test context is expected - #31

startHarper, setupHarperWithFixture, killHarper and teardownHarper all take the test context and reach the instance through ctx.harper. Passing the node instead was silently accepted by every one of them. The objects published as ctx.harper now carry a module-private, non-enumerable symbol, and all four entry points reject an argument holding it.

The teardown pair returned early on the absent ctx.harper — the same path a context whose Harper was never started takes — so the call did nothing, said nothing, and the node kept running, holding its ports, install directory and loopback slot until the runner exited. startHarper(ctx.harper) is worse and type-checks, because every field of HarperTestContext is optional: it read dataRootDir/hostname off the node (both undefined), so it built a second install directory, claimed a second loopback address, and wrote the new node to node.harper while ctx.harper still pointed at the old instance. A suite restarting per test exhausts the address pool.

An array reaches the same no-op — [ctxA, ctxB].harper is undefined — so it is rejected too, with advice to pass each context separately. An unbranded object with no harper is still a no-op, so teardown after a before hook that threw stays safe.

Found in harper-pro's decode-drop-recovery stress test, which lost both of its nodes to the teardown case for three weeks — HarperFast/harper-pro#745 fixes that call site and is the only misuse anywhere in the workspace (I scanned harper, harper-pro and symphony).

For the human reviewer

1. This is released as a major, deliberately. The commit carries a BREAKING CHANGE: footer, so semantic-release cuts 1.0.0 rather than 0.7.2. A suite that passes a node today is green-but-leaky and goes red on upgrade — that is the entire point, but it is a visible behavior change and a patch would deliver it with nothing in the release notes. The side benefit is that ^0.7.x consumers (harper-pro included) opt in on their own schedule instead of being surprised, which also removes any ordering dependency on #745. Drop the footer if you would rather ship a patch; it is a one-line change.

2. Provenance, not shape — so a copy still slips through. I first wrote this as a field-name sniff (operationsAPIURL/httpURL/dataRootDir). Review talked me out of it: the check runs only when ctx.harper is falsy, which is exactly the "before hook threw" path, so a caller-owned process, httpURL or dataRootDir would have stacked a bogus "you passed a node" error on top of the real startup failure. The brand has no false-positive class at all. Its cost is that teardownHarper({ ...ctx.harper }) keeps the enumerable process, loses the brand, and no-ops as before. Closing that would mean rejecting any object holding a live ChildProcess — reintroducing the false positive above. Same reason I did not add the symmetric assertion to sendOperation (which takes the node): requiring a brand can be wrong for a hand-built node pointed at an existing instance, whereas rejecting a brand never can.

3. Throwing can turn a leak of one into a leak of the rest. A cleanup loop — for (const c of [ctxA, node, ctxC]) await teardownHarper(c) — previously leaked only node; now node throws and ctxC never reaches its port wait, address release or rm(dataRootDir). Processes are still reaped by the process.once('exit') handler and the slot stays PID-parked, so the amplified part is disk plus a late kill, and it takes an already-misusing caller. The alternative is to recognize the branded node and tear it down anyway with a warning: no leak, but nothing fails, and a CI warning is easy to never read. I kept the throw because the silent no-op is the defect; this is the call I'd most like overruled if you disagree.

4. Both publish sites rest on inspection; the funnel they share is tested directly. Every node reaches ctx.harper through one publishHarperNode, and a test asserts that function brands what it assigns (verified load-bearing: removing the brand there fails exactly that test). I originally pinned the setupHarperWithFixture site by calling it for real, but review was right that it dragged a Harper-free suite into claiming a machine-global loopback slot and depending on scripts/setup-loopback.sh — a guard test that could go red, or hang on a saturated pool, for reasons unrelated to the guard. So a refactor that bypassed the funnel at either site would keep CI green. Reaching those lines needs a real install and address allocation, which is a change to how this suite bootstraps.

5. markHarperNode is exported from the module but not from index.ts, following the runHarperCommand precedent in AGENTS.md. It is still importable by deep path, and it is what lets the tests brand their own objects rather than going through the production sites — which is the root of point 4.

Verification

Route: the repo's full gate plus a before/after behavioral probe. CI runs exactly npm run check, npm run build, npm test; all three pass locally on Node 24.

npm run check && npm run build && npm test   # 27/27 pass, 0 fail

Behavioral proof — a full node shape (every field a started node has, plus a live child) passed unwrapped to teardownHarper:

Mutation-tested the new coverage: removing the brand from the setupHarperWithFixture publish site fails exactly one test (the node published by setupHarperWithFixture is branded) and nothing else.

Both remedy strings were read from a live run, because a generated one was actively harmful: the message is built per direction, so the start pair says "Pass the context the node came from" rather than "Wrap it" — advising a caller with a live node to wrap it would publish a fresh node over ctx.harper and abandon the running instance. A test asserts the start pair never emits Wrap it:.

A like-for-like fails-on-base run of the new tests is not constructible — they import markHarperNode, which does not exist on base, so the file fails to load rather than reaching an assertion. The probe above is the behavioral equivalent. An earlier field-sniffing iteration of these tests did run clean on base: 8 failures, all Missing expected rejection, with both no-op control tests passing on base and on the branch.

Dismissed after checking: a reported TS2367 blocker on ctx === null. Neither tsconfig sets strict/strictNullChecks and neither extends a base config, so null is assignable and the comparison has overlap; both compilers CI invokes pass. The guard parameter is typed unknown regardless, so the runtime check and the type now agree.

Not run: any real Harper start/stop cycle — this suite has no Harper to start. See point 4.

Complexity: medium

_{Review-Coverage: authored=claude; ran=gemini; adjudicated=domain; blocked=codex(exit-1); declined=cursor-grok,cursor-composer; rounds=13 @ 7fa6ee3}

_{Human-Review-Need: 4 (decisions: brand-vs-duck-typing, throw-vs-warn, release-type, shape-only-scope, one-directional, test-only-exports) @ 7fa6ee3}